Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support calling close() on cached.Database() #1407

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rhansen
Copy link
Contributor

@rhansen rhansen commented Dec 6, 2020

Introduce a refcount for cached Database entries so that calling close() on the Database returned from cached.Database() does not invalidate existing or future uses of the same cached Database.

This is a backwards-incompatible change because it changes the number of times close() must be called before the underlying Database is closed.

rhansen added a commit to ether/ueberDB that referenced this pull request Dec 7, 2020
The `sqlite3.cached.Database()` function returns a Database object
that cannot be safely closed. See:
TryGhost/node-sqlite3#1407
@rhansen rhansen force-pushed the cached-close branch 2 times, most recently from aaf95e2 to bf0d988 Compare February 16, 2021 06:05
@rhansen rhansen force-pushed the cached-close branch 2 times, most recently from 81d4683 to 92d7288 Compare August 30, 2021 07:38
Introduce a refcount for cached Database entries so that calling
close() on the Database returned from cached.Database() does not
invalidate existing or future uses of the same cached Database.

This is a backwards-incompatible change because it changes the number
of times close() must be called before the underlying Database is
closed.
Copy link

@Imran-imtiaz48 Imran-imtiaz48 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Caching Mechanism: The implementation uses a caching mechanism for SQLite database connections, which can reduce the overhead of repeatedly opening and closing database files.
  2. Reference Counting: Reference counting with refCount helps efficiently manage resources by ensuring that the database connection is only closed when it's no longer referenced. This is useful for preventing resource leaks and optimizing memory use.
  3. Callback Management: The conditional callback check ensures that the provided callback is executed appropriately, either on open or using nextTick for asynchronous consistency.

Suggestions for Improvement:

  1. Error Handling: It would be beneficial to add error handling in places where the callback is invoked to handle unexpected cases gracefully. This would be especially useful in close when super.close is called.
  2. Code Readability: To improve readability, consider renaming variables like a and b to more descriptive names. For instance, a could be flags, and b could be callback, making it easier to understand the code's purpose.
  3. Class Extraction: The Database class extension within the if (!cacheEntry) block could be extracted outside of the conditional structure, enhancing readability and potentially making it easier to test.
  4. Singleton Pattern Compliance: Since this code uses a singleton-like pattern for database instances, a further improvement would be to abstract the logic into a getOrCreateDatabaseInstance method. This would encapsulate the instance creation and retrieval logic more explicitly.

Minor Tweaks:

  • Commenting: A few more inline comments explaining the purpose of each conditional block would improve code maintainability.
  • Edge Case Testing: Make sure to test scenarios where refCount fluctuates significantly or when the cache entry is deleted during high-concurrency database usage.

Overall, this is a well-thought-out caching and resource management approach for database connections. A few adjustments would enhance the maintainability and robustness of the implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants